-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FI-3404: Add SMART on FHIR Workflow Support to CARIN Client tests #63
base: main
Are you sure you want to change the base?
Conversation
…ia SMART. Update wait test to use client id so that it can be used both for the SMART requests and resource api requests
…with new SMART workflow information
Please do this |
def input_group_prefix | ||
if test.id.include?('static') | ||
'static' | ||
elsif test.id.include?('adaptive') | ||
'adaptive' | ||
else | ||
'resp' | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is not needed in this test kit
def find_test_input(input_name) | ||
JSON.parse(result.input_json)&.find { |input| input['name'] == input_name }&.dig('value') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not needed
fhir_user: granted_scopes.include?('fhirUser'))) | ||
end | ||
|
||
response_hash.merge!(patient: '888') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this referencing a Patient on the reference server? If so I'd move it up to be another constant, so it's consistent with how you're tracking the practitioner ID.
[Click here](#{resume_claims_data_url}?token=#{access_token}) when done. | ||
[Click here](#{resume_claims_data_url}?token=#{client_id}) when done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest also updating the query parameter name since it's no longer a token. Could be client_id, or even just test_run_identifier.
|
||
def make_response | ||
client_id = extract_client_id | ||
access_token = client_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token should not be the plain client_id, because we don't want testers to see that, recognize it as the client ID, and think they can expect that from other auth servers. Instead, the client_id should be encoded in the token in some way that is opaque to the client system. In DTR I put it into a JWT, but we intend to change that, because even an encoded JWT is too recognizable. It doesn't really matter how it's encoded as long as it's simple, unrecognizable, and can be decoded.
|
||
def update_result | ||
results_repo.update(result.id, result: 'pass') unless test.config.options[:accepts_multiple_requests] | ||
JWT.encode(id_token_hash, RSA_PRIVATE_KEY, 'RS256') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're serving a signed ID token, I think you need to also expose the public key in a JWKS, e.g. https://github.com/inferno-framework/davinci-dtr-test-kit/blob/main/lib/davinci_dtr_test_kit/endpoints/mock_authorization.rb#L20
…n and address additional PR comments
Summary
Added endpoints for the SMART on FHIR authorize and token requests so that clients can now connect to this test suite via SMART.
Testing Guidance
Run the CPCDS client locally using this forked repo branch and ensure the CPCDS reference client can connect to this test suite via SMART.